Repository navigation
[FEA]: Debugger pretty-printers: cuda::annotated_ptr #10100 - #10232
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libcudacxx/test/debugging/annotated_ptr/source.cu (2)
42-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Resolve the constness of the test storage. These locals are never assigned after initialization; if the
cuda::annotated_ptr<int/float>constructors accept const pointees, declare themconst. Otherwise, document why mutable storage is required rather than silently violating the repository rule.As per coding guidelines, all variables that are not modified must be declared
const.Source: Coding guidelines
41-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winsuggestion: Add a null-pointer case to this debugger test matrix. Both printers have a dedicated
nullptrpath, but the current source only constructs non-null pointers, so regressions there will not be caught. Add the corresponding case inlibcudacxx/test/debugging/annotated_ptr/CMakeLists.txtand both expected fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 16d578af-bd9e-450c-bb12-40cc5a546ad6
📒 Files selected for processing (9)
libcudacxx/share/libcudacxx/gdb/__init__.pylibcudacxx/share/libcudacxx/gdb/annotated_ptr.pylibcudacxx/share/libcudacxx/lldb/__init__.pylibcudacxx/share/libcudacxx/lldb/annotated_ptr.pylibcudacxx/test/debugging/CMakeLists.txtlibcudacxx/test/debugging/annotated_ptr/CMakeLists.txtlibcudacxx/test/debugging/annotated_ptr/gdb.expectedlibcudacxx/test/debugging/annotated_ptr/lldb.expectedlibcudacxx/test/debugging/annotated_ptr/source.cu
iakshayubale
changed the title
iakshayubale
changed the title
iakshayubale
changed the title
4e126e5 to
b3f7119
Compare
|
pre-commit.ci autofix |
miscco
left a comment
miscco
left a comment
There was a problem hiding this comment.
Thanks a lot, this is going to be really helpful 🎉
| def public_type_name(value_type: gdb.Type) -> str: | ||
| """Return a type name without CUDA ABI inline namespaces.""" | ||
| return _ABI_NAMESPACE_PATTERN.sub("", str(value_type)) | ||
|
|
||
|
|
||
| def _template_name(value_type: gdb.Type) -> str: | ||
| return str(value_type).split("<", 1)[0] |
There was a problem hiding this comment.
Note to self, we should really split those generic helpers that are reused all the time out into a separate file
|
@iakshayubale can you rebase your branch over current |
3f6d208 to
a2e3d35
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fb37ac92-5dc4-4c5e-a761-ac86a0796676
📒 Files selected for processing (9)
libcudacxx/share/libcudacxx/gdb/__init__.pylibcudacxx/share/libcudacxx/gdb/annotated_ptr.pylibcudacxx/share/libcudacxx/lldb/__init__.pylibcudacxx/share/libcudacxx/lldb/annotated_ptr.pylibcudacxx/test/debugging/CMakeLists.txtlibcudacxx/test/debugging/annotated_ptr/CMakeLists.txtlibcudacxx/test/debugging/annotated_ptr/gdb.expectedlibcudacxx/test/debugging/annotated_ptr/lldb.expectedlibcudacxx/test/debugging/annotated_ptr/source.cu
🚧 Files skipped from review as they are similar to previous changes (6)
- libcudacxx/test/debugging/annotated_ptr/lldb.expected
- libcudacxx/test/debugging/annotated_ptr/CMakeLists.txt
- libcudacxx/share/libcudacxx/lldb/init.py
- libcudacxx/test/debugging/annotated_ptr/gdb.expected
- libcudacxx/share/libcudacxx/gdb/init.py
- libcudacxx/test/debugging/CMakeLists.txt
|
/ok to test a2e3d35 |
@Jacobfaib, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 3b91d52 |
|
pre-commit.ci autofix |
|
/ok to test c4d1b1c |
|
@Jacobfaib can we trigger the |
|
/ok to test a18f704 |
This comment has been minimized.
This comment has been minimized.
a18f704 to
589817d
Compare
|
Analysis of issue encountered part of pipeline #10232 (comment)
The transcript shows every formatter case printing correctly. The stall is after the last continue, while the program runs cudaMemcpyAsync and stream.sync(). It looks like a flaky one. Could we re-run this job? I can't re-run i see due to access rigths
GCC 16 defaults to C++20, which CMake 3.18.0 can't handle for CUDA (CMAKE_CUDA20_EXTENSION_COMPILE_OPTION not set). It was fixed on main by #12008, which merged after the previous run started. It also failed on unrelated PRs #12007 and #11869 in the same window.
Fails with rmm::cuda_stream having no member view, an RMM 26.12 API change in the external cuOpt build. This is a known failure on main.
Link errors like undefined reference to cusolverDnZungqr@libcusolver.so.12 from libcutensornet.so, a toolkit mismatch in the external cuQuantum dependency. This is a known failure on main. @Jacobfaib can we retrigger the test for 1st and 2nd above cases?, rest 3rd and 4th issues are known on main i see. |
| @@ -0,0 +1,12 @@ | |||
| =============== annotated_ptr.streaming begin =============== | |||
| (const streaming_annotated_int &) <address> cuda::annotated_ptr<int, cuda::access_property::streaming> -> <address> | |||
There was a problem hiding this comment.
This should say cuda::annotated_ptr, not the type alias. We can them omit printing the type again after it and just do -> <address> like with gdb
> Add GDB pretty-printer for cuda::annotated_ptr<_Tp, _Property> * Detects annotated_ptr types with 2 template arguments * Extracts pointee type and access property information * Displays dereferenced pointer value * Formats output as: cuda::annotated_ptr<PointeeType, PropertyType> -> 0xaddress > Add LLDB pretty-printer with equivalent functionality * Uses LLDB SBValue API for safe member and pointer access * Regex pattern matching for robust type detection * Handles edge cases (nullptr, invalid pointers) > Add comprehensive test suite * Test source with three access property scenarios (streaming, persisting, global) * Two inspection points for each property type * Expected output validation files for both GDB and LLDB * CMake integration with test discovery and execution Fixes NVIDIA#10100
> Refactor template argument extraction into _template_arguments() helper method for improved code clarity and error isolation > Refactor pointer value extraction into _pointer_display() helper method to separate concerns and improve maintainability > Extract LLDB helper functions _annotated_ptr_type_name() and _annotated_ptr_repr_value() to consolidate type and member access logic > Implement explicit optional return types (str | None) for robust error handling > Separate error handling into focused try-except blocks for each operation > Add comprehensive docstrings This resolves PR review comments regarding code duplication, error handling specificity.
> Fix type name duplication: use template_name() to extract base name without template arguments > Read template arguments from canonical_type_obj to properly handle typedef aliases > Update test fixture to use mutable backing objects (non-const int/float) > Replace nullptr constructor with default constructor for null_ptr This ensures correct GDB pretty printer output and compiling fixture values.
> Our cases had 4 arguments with extraneous 1 values that were being parsed as the section name causing: run_pretty_printer_test.py: error: duplicate section name: 1 > Fixed to match following pattern: --case inspect_streaming annotated_ptr.streaming streaming_ptr This change shall fix all 13 cascading LLDB test failures in the pipeline.
- The LLDB test was trying to evaluate local variables (streaming_ptr, persisting_ptr, global_ptr, null_ptr) that are not in scope when the debugger stops inside the inspect_* functions. Those functions only have a parameter named 'ptr' in their scope. - Changed all case expressions to evaluate 'ptr' instead of the original variable names, matching the parameter used by each inspect_* function.
- LLDB's dwim-print outputs the parameter type and address when evaluating a const-reference parameter. Update the expected output format to match the actual LLDB output: Before: cuda::annotated_ptr<...> -> <address> After: (const type_name &) <address> cuda::annotated_ptr<...> -> <address> - This aligns the LLDB test expectations with the normalized output convention used by neighboring debugger tests.
- LLDB: print only the pointer in the summary; the test now uses the full cuda::annotated_ptr type so LLDB renders it instead of a type alias. - Use the KEEP_FOR_DEBUGGER macro like the other debugger tests. - Add a before/after update case that goes from a pointer to nullptr. - Drop the blanket try/except from the LLDB and GDB printers.
5c5e682 to
e8f3948
Compare
|
@Jacobfaib have addressed the review comments, can we have a look at the changes once available. thanks. |
|
/ok to test |
@Jacobfaib, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test e8f3948 |
🥳 CI Workflow Results🟩 Finished in 1h 20m: Pass: 100%/147 | Total: 1d 06h | Max: 1h 11m | Hits: 99%/556987See results here. AI failure analysis1. RAFT mdarray test calls unavailable RMM exec_policy default constructor · 2 jobsExplanation: Both RAPIDS matrices fail at the identical RAFT test expression because the checked-out RMM exec_policy requires constructor arguments while RAFT calls it with none; the subsequent thrust::for_each_n error is derivative. The PR changes only libcudacxx debugger printers and tests, while this CI configuration independently clones RAPIDS main branches, indicating a RAPIDS snapshot compatibility failure rather than the submitted code. Evidence: Copy this prompt into a coding agentJobs: 2. MatX SAR backprojection implicit float conversion under warnings-as-errors · 1 jobExplanation: CUDA 13.4 diagnoses the implicit conversion of phase_correction_partial * w to the float argument required by __sincosf, and MatX promotes all warnings to errors. The same source expression fails across the SAR benchmark, example, and test instantiations; the submitted debugger-only diff does not touch this code. Evidence: Copy this prompt into a coding agentJobs: |
|
Thank you for the contribution! |
|
@Jacobfaib thanks for the feedbacks and proactive testing support provided:). |
Description
This PR adds comprehensive debugger pretty-printer support for
cuda::annotated_ptr<_Tp, _Property>types to libcudacxx, enabling developers to inspect pointer values and access properties in both GDB and LLDB debuggers with enhanced readability.Changes
GDB Pretty-Printer (
libcudacxx/share/libcudacxx/gdb/annotated_ptr.py)cuda::annotated_ptrtypes by examining template arguments__reprmember containing the wrapped pointer valuecuda::annotated_ptr<Type, Property> -> 0xaddressLLDB Pretty-Printer (
libcudacxx/share/libcudacxx/lldb/annotated_ptr.py)Test Suite (
libcudacxx/test/debugging/annotated_ptr/)streaming_annotated_intwithcuda::access_property::streamingpersisting_annotated_intwithcuda::access_property::persistingglobal_annotated_floatwithcuda::access_property::globalstreaming_annotated_intwithnullptrto validate null pointer handlinglibcudacxx_add_pretty_printer_testmacroRegistrations
libcudacxx/share/libcudacxx/gdb/__init__.pyto import and register the new pretty-printerlibcudacxx/share/libcudacxx/lldb/__init__.pyto import and register the new formatterlibcudacxx/test/debugging/CMakeLists.txtto include annotated_ptr tests in the test suiteCloses #10100
Checklist